Added copy to clipboard button for console tab commands#2431
Added copy to clipboard button for console tab commands#2431aadeina wants to merge 3 commits intodjango:mainfrom
Conversation
|
This looks nice 🌻 I'd recommend installing git hooks (you can follow the last section of the readme) and rebasing the PR to have a bit more clear history. |
ad9d2d6 to
24e3b21
Compare
bb03309 to
07c155d
Compare
c22de90 to
cd6cf4c
Compare
|
Hi @ulgens , |
adamzap
left a comment
There was a problem hiding this comment.
Here are some initial thoughts. Thanks for attempting this!
| }); | ||
|
|
||
| function on_success(el) { | ||
| function on_success() { |
There was a problem hiding this comment.
Why add an unused argument here and on line 145?
There was a problem hiding this comment.
Good catch! Just to clarify — I actually removed the unused el parameter here, since it wasn't being used (the functions access success_el via closure instead). Happy to keep it if you prefer consistency with other patterns in the codebase.
There was a problem hiding this comment.
Oh, thanks! Sorry for my misunderstanding here.
| // Console tabs: extract text excluding prompts (.gp) and output (.go) | ||
| const pre_el = console_section.querySelector('.highlight pre'); | ||
| text = ''; | ||
| if (pre_el) { |
There was a problem hiding this comment.
In what case is there not a <pre> element?
There was a problem hiding this comment.
You're right — since Pygments always generates the <pre> element, this check isn't strictly necessary.
I'll remove it to keep the code cleaner. Thanks for pointing this out!
| const pre_el = console_section.querySelector('.highlight pre'); | ||
| text = ''; | ||
| if (pre_el) { | ||
| pre_el.childNodes.forEach(function (node) { |
There was a problem hiding this comment.
Do you think it would be more elegant to grab the text content like the old code was doing and just left trim the prompt?
(...).textContent.trim().replace(/^\$ /, '')There was a problem hiding this comment.
Great question! I went with the node-based approach mainly because:
- It also handles the Windows tab (which uses
...>prompts) - It excludes command output marked with
.go(in case any examples show output)
That said, if the console blocks only ever show simple $ prompts without output, a regex would definitely be simpler. I'm happy to switch to:
text = pre_el.textContent.trim().replace(/^(\$ |\.\.\.>)/gm, '');There was a problem hiding this comment.
Let me know which approach you'd prefer!
There was a problem hiding this comment.
Let's see how the regex looks for now. I should have more time to look at this soon. Thanks!
There was a problem hiding this comment.
I've updated to use the regex approach. Thanks!
cd6cf4c to
b3cf678
Compare
|
Updated per feedback — removed the |
|
Thank you. In the future, could you wait to squash until the PR is ready? That way I can review new commits in isolation. |
|
Understood, apologies for the premature squash. I’ll keep commits separate going forward and appreciate the guidance on best practices. |
adamzap
left a comment
There was a problem hiding this comment.
Thanks for the changes here. I'm seeing two small design issues with the "Copied!" text on console tabs:
- I think the text should use our monospace font to match the "Copied!" text on code snippets
- The text is changing the layout vertically, maybe one pixel, when it is present
Would you be able to address these?
| document.querySelectorAll('.btn-clipboard').forEach(function (el) { | ||
| el.addEventListener('click', function () { | ||
| // Remove any existing success message first | ||
| const existing = this.querySelector('.clipboard-success'); |
There was a problem hiding this comment.
Could we call this success_el and drop the comment? I guess we'd need a let.
There was a problem hiding this comment.
On second thought, I think existing_el or existing_success_el would be fine if you don't want to use the same variable.
There was a problem hiding this comment.
Agreed. Renamed to existing_el for consistency.
| if (console_section) { | ||
| // Console tabs: extract text excluding prompts | ||
| const pre_el = console_section.querySelector('.highlight pre'); | ||
| text = pre_el.textContent.replace(/^\$ |^\.\.\.\\>/gm, ''); |
There was a problem hiding this comment.
Could we set the regex to a variable and drop the comment here and in the else below? Maybe prompt_regex?
There was a problem hiding this comment.
Good call. Extracted this to const prompt_regex.
| } | ||
|
|
||
| navigator.clipboard.writeText(text).then(on_success, on_error); | ||
| navigator.clipboard.writeText(text.trim()).then(on_success, on_error); |
There was a problem hiding this comment.
The .trim() prevents a trailing newline, so the command doesn't execute immediately upon paste.
|
Visual comparison (1px layout shift fix): Before: LayoutShiftBefore.mp4After: LayoutShiftAfter.mp4Please let me know if there's anything else you'd like me to adjust! |
|
Thanks for the changes. I'll try to get to this by end of tomorrow! |
adamzap
left a comment
There was a problem hiding this comment.
👍 Looks great!
Thanks for your patience here. I will ask someone on the team to do a CSS review.
|
Thanks so much for the thorough reviews and guidance, @adamzap! I really appreciate the time you took to help refine this. Looking forward to the CSS review. |
Summary
Adds a copy-to-clipboard button to console tab code blocks (Unix / Windows installation commands).
Fixes: #1276, #2399
Builds on feedback from @sabderemane in PR #1434.
Changes
JavaScript
djangoproject/static/js/djangoproject.js.gp— e.g.$,...>).go)SCSS
djangoproject/scss/_console-tabs.scssposition: relativeto console tab sections.btn-clipboardstyling using absolute positioning (nofloat)Visual Comparison
Before
After
Video.Project.mp4